chore!: update to JS SDK 2.x #2738
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- instr-bunyan and examples/bunyan
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2738 +/- ##
==========================================
- Coverage 91.86% 91.73% -0.14%
==========================================
Files 171 166 -5
Lines 8172 8103 -69
Branches 1660 1546 -114
==========================================
- Hits 7507 7433 -74
- Misses 665 670 +5
🚀 New features to boost your workflow:
|
drop CI unit-testing of versions below 18.19.0
github-actions
bot
requested review from
blumamir,
dyladan,
legendecas and
pichlermarc
github-actions
bot
added
pkg:instrumentation-ioredis
pkg:instrumentation-mysql
pkg:instrumentation-mysql2
pkg:resource-detector-alibaba-cloud
pkg:resource-detector-aws
pkg:resource-detector-gcp
pkg:resource-detector-instana
pkg:propagation-utils
pkg:instrumentation-amqplib
pkg:instrumentation-lru-memoizer
labels
trentm
commented
Mar 10, 2025
Contributor
Author
There was a problem hiding this comment.
Reviewer note: The updates to this file are just from the linter doing style changes. Normally .js files in the repo are not processed by npm run lint.
Member
|
Thanks for working on this! As discussed offline, let's update this or do a follow-up PR with some basic upgrade/migration notes for the resource detector change. |
The package-lock update also includes an update of grcp-js (1.12.6 -> 1.13.0) https://github.com/grpc/grpc-node/releases/tag/%40grpc%2Fgrpc-js%401.13.0
Contributor
Author
Added here: open-telemetry/opentelemetry-js#5535 |
david-luna
approved these changes
Mar 12, 2025
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-gcp/src/detectors/GcpDetector.ts
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-github/src/detectors/GitHubDetector.ts
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-document-load/test/documentLoad.test.ts
Outdated
Show resolved
Hide resolved
This shows there are some more test failures in instr-document-load to deal with.
…ing.secureConnectionStart changed in sdk-trace-web
pichlermarc
approved these changes
Mar 14, 2025
Member
pichlermarc
left a comment
pichlermarc
left a comment
There was a problem hiding this comment.
Thank you for working on this. 🙌
I pinged two component owners for their input on not exporting the detector classes anymore. In general I agree with that change and it also aligns them with the other detector packages. I think there's no benefit to having the classes exported. For the AWS detectors, which are stable, this is likely a good time to do this as dropping support for old Node.js versions means this will bump it to 2.0 anyway.
| AwsBeanstalkDetectorSync, | ||
| awsBeanstalkDetectorSync, | ||
| } from './AwsBeanstalkDetectorSync'; | ||
| export { awsBeanstalkDetector } from './AwsBeanstalkDetector'; |
Member
There was a problem hiding this comment.
While I agree with dropping class exports where they're not needed, I'm wondering: was this dropped intentionally?
cc @jj22ee: what's your preference - should the class export be kept? An end-user will likely just use the constant.
Contributor
Author
There was a problem hiding this comment.
It was intentional, yes, for the reasons you gave above:
- The detector instance, e.g.
awsBeanstalkDetector, is the thing users will want to use. - There is some precedent from the core repo's detector classes also having been dropped in the 2.0.0 release or earlier.
- I don't know of a use case for exporting the class.
- We have a breaking change version bump already for dropping Node 14 and 16 support, so it was an opportunity for this breaking change.
However, I don't feel strongly, so I'm okay to re-add exporting these if others feel we should.
Member
There was a problem hiding this comment.
Thanks for clarifying :)
I agree that we should drop class exports. It's also easy to add back later in a feature release if needed 👍
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Outdated
Show resolved
Hide resolved
| * limitations under the License. | ||
| */ | ||
| export * from './ContainerDetector'; | ||
| export { containerDetector } from './ContainerDetector'; |
Member
There was a problem hiding this comment.
same here, the class used to be exported, but the end-user never really needs to be a class. I'm fine with this change.
cc @abhee11 (component owner)
david-luna
reviewed
Mar 17, 2025
| } = await this._fetchIdentity(); | ||
| const hostname = await this._fetchHost(); | ||
| _getAttributes(): DetectedResourceAttributes { | ||
| const dataP = Promise.all([this._fetchIdentity(), this._fetchHost()]); |
Contributor
There was a problem hiding this comment.
running some tests in Elastic's distribution I got an error of an unhandled promise rejection
node ./test/agent-metadata.test.js
TAP version 13
# agent metadata
# agent metadata default case
not ok 1 exited successfully: err=Error: Command failed: /Users/david/.nvm/versions/node/v18.19.0/bin/node ./fixtures/use-http-get.js node:internal/process/promises:288 triggerUncaughtException(err, true /* fromPromise */); ^ Error: ECS metadata api request timed out. at Timeout._onTimeout (/Users/david/Documents/repos/otel/opentelemetry-js-contrib/detectors/node/opentelemetry-resource-detector-alibaba-cloud/build/src/detectors/AlibabaCloudEcsDetector.js:91:29) at listOnTimeout (node:internal/timers:569:17) at process.processTimers (node:internal/timers:512:7) Node.js v18.19.0
---
operator: error
at: done (/Users/david/Documents/repos/el/elastic-otel-node/packages/opentelemetry-node/test/testutils.js:648:35)
stack: |-
Error: Command failed: /Users/david/.nvm/versions/node/v18.19.0/bin/node ./fixtures/use-http-get.js
node:internal/process/promises:288
triggerUncaughtException(err, true /* fromPromise */);
^
Error: ECS metadata api request timed out.
at Timeout._onTimeout (/Users/david/Documents/repos/otel/opentelemetry-js-contrib/detectors/node/opentelemetry-resource-detector-alibaba-cloud/build/src/detectors/AlibabaCloudEcsDetector.js:91:29)
at listOnTimeout (node:internal/timers:569:17)
at process.processTimers (node:internal/timers:512:7)
Node.js v18.19.0
at ChildProcess.exithandler (node:child_process:422:12)
at ChildProcess.emit (node:events:517:28)
at maybeClose (node:internal/child_process:1098:16)
at ChildProcess._handle.onexit (node:internal/child_process:303:5)
...
# stdout:
# |{"name":"elastic-otel-node","level":30,"preamble":true,"distroVersion":"0.7.0","env":{"os":"darwin 24.3.0","arch":"arm64","runtime":"Node.js v18.19.0"},"msg":"start Elastic Distribution of OpenTelemetry Node.js","time":"2025-03-17T12:12:47.063Z"}
# |{"name":"elastic-otel-node","level":50,"msg":"Accessing resource attributes before async attributes settled","time":"2025-03-17T12:12:47.074Z"}
# |{"name":"elastic-otel-node","level":50,"msg":"Accessing resource attributes before async attributes settled","time":"2025-03-17T12:12:47.074Z"}
# |{"name":"elastic-otel-node","level":50,"msg":"Accessing resource attributes before async attributes settled","time":"2025-03-17T12:12:47.074Z"}
# |{"name":"elastic-otel-node","level":50,"msg":"Accessing resource attributes before async attributes settled","time":"2025-03-17T12:12:47.074Z"}
# |{"name":"elastic-otel-node","level":50,"msg":"Accessing resource attributes before async attributes settled","time":"2025-03-17T12:12:47.074Z"}
# |client response: 200 {
# | date: 'Mon, 17 Mar 2025 12:12:47 GMT',
# | expires: '-1',
# | 'cache-control': 'private, max-age=0',
# | 'content-type': 'text/html; charset=ISO-8859-1',
# | 'content-security-policy-report-only': "object-src 'none';base-uri 'self';script-src 'nonce-8TGjP3Y6JPSYBgb6x6Gcsg' 'strict-dynamic' 'report-sample' 'unsafe-eval' 'unsafe-inline' https: http:;report-uri https://csp.withgoogle.com/csp/gws/other-hp",
# | server: 'gws',
# | 'x-xss-protection': '0',
# | 'x-frame-options': 'SAMEORIGIN',
# | 'set-cookie': [Array],
# | 'accept-ranges': 'none',
# | vary: 'Accept-Encoding',
# | connection: 'close',
# | 'transfer-encoding': 'chunked'
# |}
# |client response: end
# stderr:
# |node:internal/process/promises:288
# | triggerUncaughtException(err, true /* fromPromise */);
# | ^
# |
# |Error: ECS metadata api request timed out.
# | at Timeout._onTimeout (/Users/david/Documents/repos/otel/opentelemetry-js-contrib/detectors/node/opentelemetry-resource-detector-alibaba-cloud/build/src/detectors/AlibabaCloudEcsDetector.js:91:29)
# | at listOnTimeout (node:internal/timers:569:17)
# | at process.processTimers (node:internal/timers:512:7)
# |
# |Node.js v18.19.0
# skip checkTelemetry because process errored out
1..1
# tests 1
# pass 0
# fail 1In this detector and other that are doing async operations we have promises that need to be handled if they reject. I think is because now the detectResources method from @opentelemetry/resources does defer the handling of the rejected ones. Checking ResourceImpl constructor we see values are kept but not handled until waitForAsyncAttributes is called (I think in exporters).
We could solve it by being more eager on handling the async attributes in ResourceImpl or by handling all possible rejections in each detector. Doing the later means we should add a second callback function for each promised value in the attribs object. Something like
const resolveUndef = () => undefined;
return {
[SEMRESATTRS_CLOUD_PROVIDER]: dataP.then(
() => CLOUDPROVIDERVALUES_ALIBABA_CLOUD,
resolveUndef
),
[SEMRESATTRS_CLOUD_PLATFORM]: dataP.then(
() => CLOUDPLATFORMVALUES_ALIBABA_CLOUD_ECS,
resolveUndef
),
// Data from _fetchIdentity()
[SEMRESATTRS_CLOUD_ACCOUNT_ID]: dataP.then(
data => data[0]['owner-account-id'],
resolveUndef
),
[SEMRESATTRS_CLOUD_REGION]: dataP.then(data => data[0]['region-id']),
[SEMRESATTRS_CLOUD_AVAILABILITY_ZONE]: dataP.then(
data => data[0]['zone-id']
),
[SEMRESATTRS_HOST_ID]: dataP.then(
data => data[0]['instance-id'],
resolveUndef
),
[SEMRESATTRS_HOST_TYPE]: dataP.then(
data => data[0]['instance-type'],
resolveUndef
),
// Data from _fetchHost()
[SEMRESATTRS_HOST_NAME]: dataP.then(
data => data[1],
resolveUndef
),
};
Contributor
Author
There was a problem hiding this comment.
@david-luna Thanks! I'll have to update most of the res detectors in this PR for this.
Some notes:
- I think there is effectively a regression in
@opentelemetry/[email protected]here. It doesn't adequately guard against a rejection in async resource attribute values anymore. I'll raise an issue (and perhaps PR) for this. - In the meantime, for a Resource Detector to use
@opentelemetry/[email protected], the burden is now on the Resource Detector author to never have its async attribute promises reject. - I'm really curious why tests didn't catch this: some or all of tests for
@opentelemetry/resources,@opentelemetry/resource-detector-*, and@opentelemetry/auto-instrumentations-nodeshould catch this. My total guess is that there are no out-of-process tests for these, and that mocha has someuncaughtExceptionorunhandledRejectionprocess event handler that accidentally traps these. - I think the section in the migration guide (https://github.com/open-telemetry/opentelemetry-js/blob/main/doc/upgrade-to-2.x.md#-opentelemetryresources-changes-for-implementors-of-resource-detectors) should be updated to cope with this.
Contributor
Author
There was a problem hiding this comment.
- I'm really curious why tests didn't catch this: some or all of tests for
@opentelemetry/resources,@opentelemetry/resource-detector-*, and@opentelemetry/auto-instrumentations-nodeshould catch this.
@opentelemetry/resource-detector-*: At least for@opentelemetry/resource-detector-alibaba-cloudall the tests usingdetectResourceswould immediately (in the same tick) callresource?.waitForAsyncResources(), which results in the asynctry/catchin that method guarding against promise rejections in the async attrs.
I have a coming test case that adds this before the resource?.waitForAsyncResources():
import { setTimeout as setTimeoutP } from 'timers/promises';
// This pause simulates the delay between `detectResources` and
// `waitForAsyncAttributes` typically called later in an exporter.
await setTimeoutP(200); // Hope this is enough time to get error response.This triggers the error.
@opentelemetry/auto-instrumentations-node: Itsregister.test.tsdoes do an out of process test. However, it setsOTEL_NODE_RESOURCE_DETECTORS: 'none',hence no testing of async resource detection. With the following diff, the test fails:
--- a/metapackages/auto-instrumentations-node/test/register.test.ts
+++ b/metapackages/auto-instrumentations-node/test/register.test.ts
@@ -33,7 +33,6 @@ function runWithRegister(path: string): PromiseWithChild<{
timeout: 1500,
killSignal: 'SIGKILL', // SIGTERM is not sufficient to terminate some hangs
env: Object.assign({}, process.env, {
- OTEL_NODE_RESOURCE_DETECTORS: 'none',
OTEL_TRACES_EXPORTER: 'console',
OTEL_LOG_LEVEL: 'debug',
Contributor
Author
There was a problem hiding this comment.
- I think there is effectively a regression in
@opentelemetry/[email protected]here. It doesn't adequately guard against a rejection in async resource attribute values anymore. I'll raise an issue (and perhaps PR) for this.
I've opened open-telemetry/opentelemetry-js#5539 for this.
…eject Also tweak the auto-instrumentations-node/register.js test to actually use resource detectors, to help test if there are issues with them. Refs: open-telemetry/opentelemetry-js#5539
pichlermarc
approved these changes
Mar 18, 2025
legendecas
approved these changes
Mar 18, 2025
Merged
trentm
added a commit
to trentm/opentelemetry-js-contrib
that referenced
this pull request
Mar 18, 2025
…g now that target is es2022 As of open-telemetry#2738 the tsconfig target is es2022 now, so this special case is no longer necessary.
deejay1
pushed a commit
to deejay1/opentelemetry-js-contrib
that referenced
this pull request
Apr 14, 2025
Co-authored-by: Marc Pichler <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@opentelemetry/*packages from opentelemetry-js.gitper https://github.com/open-telemetry/opentelemetry-js/blob/main/doc/upgrade-to-2.x.md
^18.19.0 || >=20.6.0. This means that support for Node.js 14 and 16 has been dropped.ContainerDetectorclass is no longer exportedawsBeanstalkDetectorSync-> useawsBeanstalkDetectorawsEc2DetectorSync-> useawsEc2DetectorawsEcsDetectorSync-> useawsEcsDetectorawsEksDetectorSync-> useawsEksDetectorawsLambdaDetectorSync-> useawsLambdaDetectorAwsBeanstalkDetector,AwsEcsDetector,AwsEksDetector,AwsLambdaDetector,note to revewiers
This is ready for review. The remaining failing test will be fixed when there is a new release of 2.x/0.200.x from the core repo with open-telemetry/opentelemetry-js#5524 (see the checklist below).
checklist
instrumentation-document-loadtests passing2.0.0/0.200.0release once that is out